-
Notifications
You must be signed in to change notification settings - Fork 15k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: remove same-tag notifications before showing new ones #24302
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't expected behavior, we don't want to dismiss the previous notification before showing a new one.
The line you reference here -->
Close(); |
Notification
instances in the main process.
In other words, this:
This ensures that, like we do in the main API
Is wrong
And we don't want to do this:
we explicitly close the most recent notification from the Notification Center on macOS before creating and displaying a new one.
as it isn't in any way expected, we want to have the ability to stack multiple notifications in the notification center.
d681e19
to
c4e3e0c
Compare
c4e3e0c
to
38fb1c7
Compare
38fb1c7
to
1d255e3
Compare
faede9c
to
91ce7de
Compare
91ce7de
to
650bdc5
Compare
Release Notes Persisted
|
I have automatically backported this PR to "10-x-y", please check out #24404 |
I have automatically backported this PR to "8-x-y", please check out #24405 |
I have automatically backported this PR to "9-x-y", please check out #24406 |
I think this PR introduced an old bug, explained in this issue: #17706 Quick tldr: when showing a new notification with the same tag as the one before both the newly created notification and the one before gets it's onclose callback called and the user can't click on the notification (the onclick handler won't fire). |
Description of Change
Closes #24290.
Fixes an issue where notifications where not being closed properly when more than one notification was created in the renderer process with the same tag.
Per spec: https://notifications.spec.whatwg.org/#showing-a-notification
Tested with https://gist.github.com/fd16a663434667d2096cd31ec7f2662f.
cc @MarshallOfSound @zcbenz
Checklist
npm test
passesRelease Notes
Notes: Fixed an issue where some old notifications were not properly removed from the Notification Center on macOS.